-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Case order bug #8748
Case order bug #8748
Conversation
a739b8f
to
0e1841b
Compare
Codecov Report
@@ Coverage Diff @@
## master #8748 +/- ##
============================================
+ Coverage 62.81% 69.64% +6.82%
- Complexity 4556 4613 +57
============================================
Files 1679 1730 +51
Lines 88125 90321 +2196
Branches 13198 13424 +226
============================================
+ Hits 55354 62900 +7546
+ Misses 28801 23059 -5742
- Partials 3970 4362 +392
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ab1c2b5
to
7114733
Compare
7114733
to
2af9c00
Compare
_elseThenStatements.add(arguments.get(i)); | ||
} | ||
_selections = new boolean[_elseThenStatements.size()]; | ||
Collections.reverse(_elseThenStatements); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment here to indicate the reasoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, I'll follow up with cosmetic enhancements but want to get this merged quickly given the nature of the bug
A bug was introduced in #8138 which eliminated the evaluation of never taken then statements, however, it also reversed the case order of precedence. This fixes this regression, but also adds a test to assert that correct case precedence is observed.
Fixes #8747